Conversation
Introduce collections.Handle with Data and Query namespaces
MockTransport can now be created with stubs that describe different request-response pairs by parameterizing them with [any, any]. The Do method relies on reflection for detecting such cases and treats them differently both when comparing the request values and assigning responses back to dest.
…HEAD-like requests
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Secrets | View in Orca |
- NewClient - NewLocal - NewWeaviateCloud
| Name string | ||
| Description string | ||
| DataType DataType | ||
| NestedProperties []Property |
There was a problem hiding this comment.
note: A NestedProperty is necessarily different to a Property since you can't set inverted indices nor tokenization on a nested property. Perhaps you're aiming defensively here in case that changes?
There was a problem hiding this comment.
I haven't thought about that before to be honest. Planning for future change is a reasonable objective (I can imagine how we might allow building some inverted inverted index on the object property in the future), but my main intention here was simplicity.
you can't set inverted indices nor tokenization on a nested property
I'd argue that a separate NestedProperty type will over-specify the API. Consider: IndexRangeFilters is only applicable to int, number, and date property types, and Tokenization cannot be set for anything other than text and text[] properties. Still, we do not provide distinct ArithmeticProperty, TextProperty and BlobProperty in the spirit of keeping things simple and, as you've suggested, flexible.
There was a problem hiding this comment.
I'd argue that a separate NestedProperty type will over-specify the API
We are currently planning filtering+search support for NestedProperty and there is a good chance that some config values will be different than normal Properties. I think for this case it makes sense to have an extra type
There was a problem hiding this comment.
config values will be different than normal Properties
Do you mean like Tokenization field would exist but would accept a different value? E.g. it's string for text/text[] properties but []bool for object/object[]? If that's the case, then I agree that this warrants a separate type.
I don't think it is necessary otherwise. If we take a close look a the current property config options, most of the properties aren't "normal".
// Applicable to all property types without exception
Name string
Description string
DataType DataType
// Exclusive to `object` and `object[]`
NestedProperties []Property
// Exclusive to `text` and `text[]`
Tokenization Tokenization
IndexSearchable bool
// Exclusive to `int`, `number`, and `date`
IndexRangeFilters bool
// Not applicable to `blob`, `object`, `geoCoordinates`, `phoneNumber`
IndexFilterable boolOnly 4/8 configurations are applicable to "most" property types.
There was a problem hiding this comment.
It also makes the API less consistent. Consider an object property address { street, building_nr }:
NestedProperties: []Property{
{Name: "street", DataType: "text", IndexSearchable; true}, // Invalid -- NestedProperty would fix that
{Name: "building_nr", DataType: "int"},
}The issue with using Property directly is I can specify IndexSearchable which is a mistake -- the NestedProperty simply wouldn't expose that config. What if in the future we decide to make nested text searchable but not text[]? We'll extend NestedProperty with a new field and end up where we started, i.e. the user can accidentally send an invalid request.
NestedProperties: []NestedProperty{
{Name: "one": DataType: "text", IndexSearchable: true}, // OK
{Name: "many": DataType: "text[]", IndexSearchable: true}, // Invalid
},There was a problem hiding this comment.
Do you mean like Tokenization field would exist but would accept a different value? E.g. it's string for text/text[] properties but []bool for object/object[]? If that's the case, then I agree that this warrants a separate type.
We are still in the planning phase so I cant tell you exactly what we have, but we might end up with different options for IndexRangeable than normal Properties. As we are quite far away from GA I am also ok with leaving it as normal Property for now with the option of changing it in the future (I haven't read the complete thread)
There was a problem hiding this comment.
Something to note as well is that Property and NestedProperty are different types in the OpenAPI schema too so removing that point of difference in the client would be a conscious choice to differ from the server (not necessarily a bad thing, just pointing out)
v6: Create a new client instance
This PR adds the lower-level requests and the public API for managing Weaviate collections: create, get, list, exists, delete, and delete all.
We also introduce
collections.Handlecontainer for.Data,.Query, and other namespaces, and functional options to configure set tenant / consistency level for the handle.Key changes:
internal/api/collections.go- requests and endpoint implementationscollections/client.go- public API, the "collections" namespaceinternal/api/exists.go- universaldesttype for HEAD-like requests (where the body is returned, but we can discard it)internal/transports/rest.go- new Endpoint utilities: StaticEndpoint and IdentityEndpointinternal/api/endpoint_test.go,internal/api/exists_test.go,internal/transports/rest_test.goandcollections/client_test.go- supporting unit testsMiscellaneous:
testkit.MockTransportcan now handle heterogenous request-response stubs. This gives the developer to test a public API method, which may make several different "transport calls" to execute. SeeTestClient_DeleteAllfor an illustrative example.